refactor: extract deduplication logic from `AddRemoveDedupVisitor` into embeddable `FileActionsDeduplicator` #769

sebastiantia · 2025-03-26T21:17:44Z

What changes are proposed in this pull request?

No behavioral changes were introduced, this is purely a refactoring effort

This PR extracts the core deduplication logic from the AddRemoveDedupVisitor in order to be shared with the incoming CheckpointVisitor. For a bigger picture view on how this refactor is helpful, please take a look at the following PR which implements the CheckpointVisitor with an embedded FileActionsDeduplicator that will rebase this PR once merged. [link to PR].

This FileActionsDeduplicator lives in the new top-level log_replay mod as it will be leveraged in the nested scan/log_replay mod and the incoming checkpoints/log_replay mod. There are also additional traits & structs that the two log_replay implementations will share via this new top-level mod. For an even wider view of the implementation of the checkpoints mod and the component re-use, please have a look at the following PR. [link to PR]

Summary of refactor

New log_replay mod
Moved FileActionKey definition from scan/log_replay to the new log_replay mod
New FileActionDeduplicator in the new log_replay mod
- Includes the check_and_record_seen method which was simply moved from the AddRemoveDedupVisitor
- Includes the extract_file_action method and extract_dv_unique_id private method which may be new concepts, but includes functionality which are both pieces of functionality pulled from the AddRemoveDedupVisitor to be shared with the incoming CheckpointVisitor

How was this change tested?

All existing tests pass ✅

codecov · 2025-03-26T22:09:28Z

Codecov Report

Attention: Patch coverage is 91.26984% with 11 lines in your changes missing coverage. Please review.

Project coverage is 84.69%. Comparing base (9290930) to head (e982960).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/log_replay.rs	89.28%	2 Missing and 7 partials ⚠️
kernel/src/scan/log_replay.rs	95.23%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #769      +/-   ##
==========================================
+ Coverage   84.64%   84.69%   +0.04%     
==========================================
  Files          82       83       +1     
  Lines       19735    19816      +81     
  Branches    19735    19816      +81     
==========================================
+ Hits        16705    16783      +78     
  Misses       2214     2214              
- Partials      816      819       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

This PR refactors the deduplication logic by extracting it from AddRemoveDedupVisitor into a standalone, embeddable FileActionDeduplicator to enable reuse with the upcoming CheckpointVisitor.

Moves deduplication logic into a dedicated FileActionDeduplicator in the new log_replay module.
Refactors AddRemoveDedupVisitor to utilize FileActionDeduplicator via index constants.
Updates the module hierarchy in lib.rs to register the new log_replay module.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
kernel/src/log_replay.rs	Introduces FileActionDeduplicator struct to encapsulate deduplication logic.
kernel/src/lib.rs	Adds the new log_replay module.
kernel/src/scan/log_replay.rs	Refactors AddRemoveDedupVisitor to use FileActionDeduplicator and its index constants.

Comments suppressed due to low confidence (1)

kernel/src/scan/log_replay.rs:43

[nitpick] Consider adding inline comments for the index constants (e.g., ADD_PATH_INDEX, ADD_PARTITION_VALUES_INDEX) to clearly explain their mapping to file action fields and reduce ambiguity in future maintenance.

const ADD_PATH_INDEX: usize = 0;

zachschuermann

generally looks good, left a few comments. can we get additional context on the material changes here? hard to see what's being changed vs what's being moved. (might even be a good idea to have a prefactor PR that just introduces the new module and moves code around as needed)

kernel/src/log_replay.rs

sebastiantia · 2025-03-26T22:29:19Z

generally looks good, left a few comments. can we get additional context on the material changes here? hard to see what's being changed vs what's being moved. (might even be a good idea to have a prefactor PR that just introduces the new module and moves code around as needed)

Makes sense, will update the PR description to add some context as to what's going on.

Talked offline, will continue with changes in this PR as the only moving of existing code is the FileActionKey struct and the AddRemoveDedupVisitor.check_and_record_seen method to the new log_replay module

zachschuermann

seems reasonable, just a couple comments and having more information on what moved vs what is material change will help reviews

kernel/src/log_replay.rs

zachschuermann · 2025-03-27T00:58:24Z

kernel/src/log_replay.rs

+    /// should be ignored). If not already seen, register it so we can recognize future duplicates.
+    /// Returns `true` if we have seen the file and should ignore it, `false` if we have not seen it
+    /// and should process it.
+    pub(crate) fn check_and_record_seen(&mut self, key: FileActionKey) -> bool {


can we call out what is just a simple move and what is a material change? AFAICT this is code that already existed?

Directly extracted from AddRemoveDedupVisitor

sebastiantia · 2025-03-27T16:45:11Z

kernel/src/log_replay.rs

+/// The subset of file action fields that uniquely identifies it in the log, used for deduplication
+/// of adds and removes during log replay.
+#[derive(Debug, Hash, Eq, PartialEq)]
+pub(crate) struct FileActionKey {


Moved from scan/log_replay.rs

zachschuermann

few comments - and if we could add comments inline in the PR changes for moves/changes that would help reviewers, sorry to harp on this still but with (1) critical changes like this in log replay and (2) code movement/changing, I think we need to do everything we can to ensure thorough/effective reviews

kernel/src/log_replay.rs

zachschuermann · 2025-03-27T18:53:23Z

kernel/src/scan/log_replay.rs

+            if self.deduplicator.selection_vector_ref()[i] {
+                self.deduplicator.selection_vector_mut()[i] = self.is_valid_add(i, getters)?;


can we bundle this up in a method that checks selection vector then does a closure or something if true (and sets selection vector to closure output?)

Great idea 👍

Tried something along the lines of:

self.deduplicator.process_selection_with(|i| self.is_valid_add(i, getters))?;

The issue is that is_valid_add takes &mut self, and the closure in process_selection_vector_with is trying to call that method, which creates a borrowing conflict.

Talked offline - selection vector does not need to be drilled into the FileActionsDeduplicator.

zachschuermann · 2025-03-27T18:54:22Z

kernel/src/scan/log_replay.rs

-        // Add will have a path at index 0 if it is valid; otherwise, if it is a log batch, we may
-        // have a remove with a path at index 4. In either case, extract the three dv getters at
-        // indexes that immediately follow a valid path index.


can we persist the comment still? seems useful to include the context on indexes

Sounds good, will make the docs a bit more clear as well

kernel/src/log_replay.rs

zachschuermann · 2025-03-27T18:56:47Z

kernel/src/log_replay.rs

+    /// * `Ok(Some((key, is_add)))` - When a file action is found, returns the key and whether it's an add operation
+    /// * `Ok(None)` - When no file action is found
+    /// * `Err(...)` - On any error during extraction
+    pub(crate) fn extract_file_action<'a>(


did we not have any specific UT on these in the old module? might be worth introducing a handful of quick tests here..

the original module didn't have specific unit tests for these functions, as they were primarily covered by integration tests at a higher level. Since this PR is focused purely on refactoring for code organization without functional changes, I'd prefer to keep tests separate to maintain a clear scope for reviewers.

sure - can we track a follow-up then?

Hello @sebastiantia and @zachschuermann, I am new to the open source community. I was told that Rust community is very supportive and helpful. I was wondering if I could try to write these tests. I joined the slack channel but the last activity there was 12 March.

👋 Hi @snackoverflow215! We certainly aim to be supportive and helpful :) - please feel free to add some tests for this, that would be great! I saw you opened #783 already, I'll take a look today

sebastiantia · 2025-03-27T21:43:06Z

kernel/src/scan/log_replay.rs

-                dv_getters[2].get_opt(i, "deletionVector.offset")?,
-            )),
-            None => None,
-        };


All of this logic is used to generate the file action key - which is now abstracted and supplied by the FileActionsDeduplicator.extract_file_action() method

sebastiantia · 2025-03-27T21:43:45Z

kernel/src/scan/log_replay.rs

-    /// should be ignored). If not already seen, register it so we can recognize future duplicates.
-    /// Returns `true` if we have seen the file and should ignore it, `false` if we have not seen it
-    /// and should process it.
-    fn check_and_record_seen(&mut self, key: FileActionKey) -> bool {


This method has been moved to the FileActionDeduplicator

sebastiantia · 2025-03-27T21:44:42Z

kernel/src/log_replay.rs

+    }
+
+    /// Extract the deletion vector unique ID if it exists.
+    fn extract_dv_unique_id<'a>(


An internal refactor, only called from extract_file_action to generate the file action key from a row of EngineData

can we document some of the requirements here (e.g. you give a dv_start_index and then it does uses getters from that index and + 1 and + 2 for extracting the storage type, dv, and offset fields.)

nicklan

generally looks good, just some small comments. thanks!

nicklan · 2025-03-27T23:11:58Z

kernel/src/scan/log_replay.rs

-                self.seen.insert(key);
-            }
-            false
+    // The index position in the row getters for the following columns


Boy we need a way to get these out of the schema and not to manually encode them :)

kernel/src/scan/log_replay.rs

nicklan · 2025-03-27T23:25:03Z

kernel/src/log_replay.rs

+    ///
+    /// # Returns
+    ///
+    /// * `Ok(Some((key, is_add)))` - When a file action is found, returns the key and whether it's an add operation


why not move the is_add inside FileActionKey? Even more rusty would be to have a quick:

enum FileActionKeyType { Add, Remove, }

and have that be part of the above struct.

If you want to avoid code changes in a refactor that makes sense, maybe just make a follow-up issue?

Good idea 👍 will track with a follow-up issue

kernel/src/scan/log_replay.rs

This reverts commit a243a98.

nicklan

lgtm! thanks

zachschuermann

very nice :) considering the sensitivity of this area i've tried to give it a thorough review! just left a few more nits/questions and would like to have us take up some of those UT additions soon

LGTM!

zachschuermann · 2025-03-28T20:27:34Z

kernel/src/log_replay.rs

+    /// * `Ok(Some((key, is_add)))` - When a file action is found, returns the key and whether it's an add operation
+    /// * `Ok(None)` - When no file action is found
+    /// * `Err(...)` - On any error during extraction
+    pub(crate) fn extract_file_action<'a>(


sure - can we track a follow-up then?

kernel/src/log_replay.rs

zachschuermann · 2025-03-29T00:21:09Z

kernel/src/log_replay.rs

+    }
+
+    /// Extract the deletion vector unique ID if it exists.
+    fn extract_dv_unique_id<'a>(


can we document some of the requirements here (e.g. you give a dv_start_index and then it does uses getters from that index and + 1 and + 2 for extracting the storage type, dv, and offset fields.)

kernel/src/log_replay.rs

zachschuermann · 2025-03-29T00:35:18Z

kernel/src/log_replay.rs

+impl<'seen> FileActionDeduplicator<'seen> {
+    pub(crate) fn new(
+        seen_file_keys: &'seen mut HashSet<FileActionKey>,
+        is_log_batch: bool,


thinking out loud: we could actually make this something like "deduplicate_batch: bool" or "save_batch: bool" ? baking in less checkpoint/commit specific logic and instead just communicating whether or not we should save the file actions in the hashmap for future deduplication (i.e. we do this in commit batches but not checkpoint batches)

let's not solve that here, perhaps talk about little rename in the future

Left a todo...
// TODO: Consider renaming to `is_commit_batch`, `deduplicate_batch`, or `save_batch` to better reflect its role in deduplication logic.

…dupVisitor

zachschuermann · 2025-04-01T18:00:43Z

kernel/src/lib.rs

 pub mod engine_data;
 pub mod error;
 pub mod expressions;
+pub mod log_replay;


just noticing (sorry this snuck through somehow) this shouldn't be pub

## What changes are proposed in this pull request?  ### Key changes resolves #737. This PR implements the `CheckpointVisitor` necessary for filtering a stream of actions into a stream of actions to be included in a checkpoint file. This leverages the `FileActionDeduplicator` [[link to PR]](#769). This PR introduces the `checkpoint` mod, and implements the visitor in the new `checkpoint/log_replay` mod. Comprehensive module documents are included in the new modules which provide an overview of the incoming code additions, along with it's goal. ### Checkpoint Content A **complete V1 checkpoint** encapsulates: 1. All FILE actions that make up the state of a version of a table: - Add actions (after action reconciliation) - Unexpired remove actions ([remove tombstones](https://github.com/delta-io/delta/blob/master/PROTOCOL.md#add-file-and-remove-file)) 2. All NON-FILE actions that make up the state of a version of a table: - Protocol action - Metadata action - Txn actions A **single-file V2 checkpoint** is simply a super-set of the actions included in the V1 checkpoint schema, with the addition of the `CheckpointMetadata` action (which must be generated on every write). Since single-file v2 checkpoints will also leverage this visitor, we have chosen to name it the general `CheckpointVisitor` Note: - CDC, CommitInfo, Sidecar, and CheckpointMetadata actions are NOT part of the **V1** checkpoint schema. - Sidecar and CheckpointMetadata actions are part of the **V2** checkpoint schema. ### The new `CheckpointVisitor` This visitor selects the **FILE** actions for a V1 spec checkpoint via a selection vector: 1. Processes add/remove actions with proper deduplication based on path and deletion vector ID pairs 2. Optimization: Only tracks already seen file paths in **commit files**, as actions in checkpoint files are the last batches to be processed, and do not conflict with other actions in checkpoint files. 3. Applies tombstone expiration logic by filtering out remove actions with deletion timestamps older than the minimum file retention timestamp This visitor also selects the **NON-FILE** actions for a V1 spec checkpoint via a selection vector: 1. Ensures exactly one protocol action is included (the newest one encountered) 2. Ensures exactly one metadata action is included (the newest one encountered) 3. Deduplicates transaction (txn) actions by app ID to include only the newest action for each app ID  ## How was this change tested?  `test_checkpoint_visitor` - Tests basic functionality with both file and non-file actions, verifying correct counts and selection vector. `test_checkpoint_visitor_boundary_cases_for_tombstone_expiration` - Tests how tombstone expiration handles threshold boundary conditions. `test_checkpoint_visitor_conflicting_file_actions_in_log_batch` - Verifies duplicate path handling in log batches (keeping first, skipping second). `test_checkpoint_visitor_file_actions_in_checkpoint_batch` - Tests that duplicate file actions are included in checkpoint batches. `test_checkpoint_visitor_conflicts_with_deletion_vectors` - Tests file deduplication with deletion vectors to ensure uniqueness. `test_checkpoint_visitor_already_seen_non_file_actions` - Verifies that pre-populated actions are skipped correctly. `test_checkpoint_visitor_duplicate_non_file_actions` - Tests deduplication of non-file actions (protocol, metadata, transactions).

sebastiantia added 2 commits March 26, 2025 14:10

refactor

a618833

more docs

7da74b2

github-actions bot assigned sebastiantia Mar 26, 2025

invert is_log_batch logic

220a216

sebastiantia requested review from nicklan and zachschuermann March 26, 2025 22:06

sebastiantia marked this pull request as ready for review March 26, 2025 22:06

sebastiantia added 2 commits March 26, 2025 15:14

docs

9d86911

docs

e5b0e32

zachschuermann requested review from Copilot and scovich March 26, 2025 22:17

Copilot AI reviewed Mar 26, 2025

View reviewed changes

zachschuermann reviewed Mar 26, 2025

View reviewed changes

kernel/src/log_replay.rs Outdated Show resolved Hide resolved

kernel/src/log_replay.rs Outdated Show resolved Hide resolved

kernel/src/log_replay.rs Show resolved Hide resolved

docs and imports

a5393dc

sebastiantia requested a review from zachschuermann March 26, 2025 22:48

zachschuermann reviewed Mar 27, 2025

View reviewed changes

improve mod doc

a23c651

sebastiantia requested a review from zachschuermann March 27, 2025 03:04

improve doc

d712d18

sebastiantia commented Mar 27, 2025

View reviewed changes

docs'

e564ae1

This was referenced Mar 27, 2025

feat: add CheckpointVisitor in new checkpoint mod #738

Merged

refactor: extract log replay processing structure into LogReplayProcessor trait #774

Merged

zachschuermann reviewed Mar 27, 2025

View reviewed changes

sebastiantia added 2 commits March 27, 2025 13:42

docs

b14ff19

update

a52d484

sebastiantia commented Mar 27, 2025

View reviewed changes

sebastiantia requested a review from zachschuermann March 27, 2025 21:48

nicklan reviewed Mar 27, 2025

View reviewed changes

sebastiantia added 3 commits March 27, 2025 16:59

nits

a243a98

Revert "nits"

9f06382

This reverts commit a243a98.

nits

58f38c0

sebastiantia requested a review from nicklan March 28, 2025 00:11

nicklan approved these changes Mar 28, 2025

View reviewed changes

zachschuermann approved these changes Mar 29, 2025

View reviewed changes

sebastiantia and others added 2 commits March 30, 2025 14:27

docs

964f294

Merge branch 'main' into extract-deduplication-logic-from-addRemoveDe…

e982960

…dupVisitor

sebastiantia merged commit 06fb373 into delta-io:main Mar 31, 2025
21 checks passed

zachschuermann reviewed Apr 1, 2025

View reviewed changes

This was referenced Apr 14, 2025

[tracking issue] checkpoint write support #499

Closed

[tracking issue] Single-file classic-named V1 & V2 checkpoint write support #736

Closed

		if self.deduplicator.selection_vector_ref()[i] {
		self.deduplicator.selection_vector_mut()[i] = self.is_valid_add(i, getters)?;

refactor: extract deduplication logic from AddRemoveDedupVisitor into embeddable FileActionsDeduplicator #769

refactor: extract deduplication logic from AddRemoveDedupVisitor into embeddable FileActionsDeduplicator #769

Conversation

sebastiantia commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are proposed in this pull request?

Summary of refactor

How was this change tested?

Uh oh!

codecov bot commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sebastiantia commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastiantia Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastiantia Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastiantia Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refactor: extract deduplication logic from `AddRemoveDedupVisitor` into embeddable `FileActionsDeduplicator` #769

refactor: extract deduplication logic from `AddRemoveDedupVisitor` into embeddable `FileActionsDeduplicator` #769

sebastiantia commented Mar 26, 2025 •

edited

Loading

codecov bot commented Mar 26, 2025 •

edited

Loading

sebastiantia commented Mar 26, 2025 •

edited

Loading

sebastiantia Mar 27, 2025 •

edited

Loading

sebastiantia Mar 27, 2025 •

edited

Loading

sebastiantia Mar 27, 2025 •

edited

Loading